-
Notifications
You must be signed in to change notification settings - Fork 35
feat: add support for Geth built-in tracers #1251
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 7d64ffc The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
703348d to
b1caf09
Compare
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1251 +/- ##
==========================================
- Coverage 74.13% 74.10% -0.03%
==========================================
Files 437 437
Lines 74716 74598 -118
Branches 74716 74598 -118
==========================================
- Hits 55389 55282 -107
+ Misses 17302 17285 -17
- Partials 2025 2031 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds support for Geth built-in tracers by updating the Hardhat dependency from version 2.26.1 to 2.28.0 and implementing corresponding changes in the EDR codebase to handle debug tracing with Geth-compatible options.
Key Changes
- Updates Hardhat to version 2.28.0 with patches to support Geth debug tracing
- Replaces custom EIP-3155 tracer implementation with
revm-inspectorsDebugInspector - Updates test fixtures to match Geth 1.16.7 output format
Reviewed changes
Copilot reviewed 22 out of 28 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updates Hardhat dependency from 2.26.1 to 2.28.0 |
| pnpm-workspace.yaml | Adds "hardhat" to immediate install exceptions |
| patches/[email protected] | Adds patch file for Hardhat 2.28.0 |
| patches/[email protected] | Removes obsolete patch for Hardhat 2.26.1 |
| crates/edr_provider/src/debug_trace.rs | Replaces custom tracer with DebugInspector from revm-inspectors |
| crates/edr_provider/src/requests/debug.rs | Updates debug trace methods to use GethDebugTracingOptions |
| crates/edr_provider/Cargo.toml | Adds dependencies for alloy-rpc-types-trace and revm-inspectors |
| hardhat-tests/test/internal/hardhat-network/provider/utils/assertEqualTraces.ts | Simplifies trace comparison logic |
| hardhat-tests/test/fixture-debug-traces/*.ts | Updates test fixtures with Geth 1.16.7 traces |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
hardhat-tests/test/fixture-debug-traces/elongatedMemoryRegressionTestTrace.ts
Show resolved
Hide resolved
hardhat-tests/test/internal/hardhat-network/provider/modules/debug/traceTransaction.ts
Show resolved
Hide resolved
|
|
||
| Our API is now aligned with Geth's tracing capabilities. | ||
|
|
||
| BREAKING CHANGE: Memory capture used to be enabled by default on geth, but has since been flipped <https://github.com/ethereum/go-ethereum/pull/23558> and is now disabled by default. We have followed suit and disabled it by default as well. If you were relying on memory capture, you will need to explicitly enable it by setting the `enableMemory` option to `true` in your tracer configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cc: @michalbrabec
|
Lint error should be resolved by #1255 |
| viaIR: true, | ||
| }, | ||
| }, | ||
| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation: To ensure that the stack trace tests keep succeeding, we need to add new solc compiler descriptions up until the version of Solidity that EDR reports as supporting.
| }); | ||
|
|
||
| describe("Solidity support", function () { | ||
| it("check that the latest tested version is within the supported version range", async function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation: When these tests lived in Hardhat, it made sense to ensure that there was a test to validate that the latest test version and the SUPPORTED_SOLIDITY_VERSION_RANGE matched.
Since these tests now live in EDR, there is no need to check that. We only need to ensure that we test all solc versions up until the one reported by EDR (latestSupportedSolidityVersion)
| it("check that the latest tested version matches the one that EDR exports", async function () { | ||
| const latestSupportedVersion = getLatestTestedSolcVersion(); | ||
| const edrLatestSupportedVersion = getLatestSupportedSolcVersion(); | ||
| const edrLatestSupportedVersion = latestSupportedSolidityVersion(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Motivation: I replaced the previously hardcoded value from getLatestSupportedSolcVersion with the dynamic value in latestSupportedSolidityVersion that reports the latest supported Solidity version of Slang.
| "ethers": "^6.1.0", | ||
| "fs-extra": "^7.0.1", | ||
| "hardhat": "2.26.1", | ||
| "hardhat": "2.28.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: The upgrade of Hardhat to 2.28.0 brought to light issues with the stack trace tests
| use std::{collections::HashMap, fmt::Debug}; | ||
| use std::fmt::Debug; | ||
|
|
||
| use alloy_rpc_types_trace::geth::{GethDebugTracingOptions, GethTrace}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we re-export these types and rename them to DebugTracingOptions and Trace to avoid filling our code with references to geth?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to. I'm fine either way.
It signals that it matches the format defined by Geth, which we consciously follow. We can also rename it to something generic though. Would you like me to change it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
whatever you feel best. It was just to bring it up in case you hadn't considered it
anaPerezGhiglia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR removes our custom EIP-3155 tracer and instead uses
revm-inspector'sDebugInspector. This brings us one step closer to unification of the tracing architecture, as we can use the same underlying inspector to generate our stack traces.During testing, I discovered a bug in
revm-inspectors. I was able to fix the bug and created a PR. After some back and forth, I'm waiting for the final approval. Until then, I decided to go ahead with a temporary dependency patch, using my fork ofrevm-inspectors. If desired, I can move the fork to the NomicFoundation organisation.The
hardhat-testsare using fixtures to validate that the generated output ofdebug_traceCallanddebug_traceTransactionmatches what Geth returns for specific requests. Unfortunately, the fixtures were old and no longer reflect the behaviour of the latest version of Geth. As such, I had to update the fixtures.For future reference, I added instructions for how to generate new fixtures for future versions of Geth to each individual fixture. The instructions slightly differ per fixture as each fixture contains different JSON-RPC requests.
For fixtures that use "forking mode" I could not use Geth, since Geth does not support the concept of forking a blockchain. It can only fully sync to the live blockchain. To avoid a lot of time and storage to sync a node, I generated fixtures for "forking mode" by calling Alchemy. As Alchemy runs a mixture of clients (e.g. Erigon, Reth, and Geth), this does run a risk of hitting an Archive node that generates its JSON-RPC responses using Reth, which uses the same
alloydependency as us. I deemed this to be an acceptable trade-off to save a lot of time.There is one outstanding issue. A previously disabled test can theoretically be re-enabled. However, I discovered a bug in REVM. I fixed the bug and created a PR. Once that's merged, I'll upgrade REVM and re-activate the test in a follow-up PR.